Repository navigation
[A11y] Connect the accessibility override controls to the app - #10065
victorsanni wants to merge 1 commit into
Conversation
Adds ViewMetricsOverrideClient, a client for ext.flutter.viewMetricsOverride, and connects the text scale, bold text, screen reader and high contrast controls of the Accessibility page to it.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for the ext.flutter.viewMetricsOverride service extension, enabling users to override accessibility settings (text scale, bold text, screen reader, and high contrast) in debug builds. It introduces ViewMetricsOverrideClient to manage the service extension, handles state restoration across hot restarts, and updates the accessibility controller and UI overrides pane. Feedback focuses on improving error handling: rethrowing and catching service extension failures to notify users, handling background errors with .catchError, adding a defensive check against division by zero in _textScaleFactor, and using a try-finally block to ensure the text scale slider resets properly on failure.
| Future<void> _call(Map<String, String> parameters) async { | ||
| final isolateGeneration = _isolateGeneration; | ||
| try { | ||
| final response = await serviceConnection.serviceManager | ||
| .callServiceExtensionOnMainIsolate( | ||
| registrations.viewMetricsOverride, | ||
| args: parameters, | ||
| ); | ||
| if (disposed || isolateGeneration != _isolateGeneration) return; | ||
| _takeOverridesFrom(response.json?['overrides']); | ||
| } catch (e, st) { | ||
| _log.warning( | ||
| 'Failed to call ${registrations.viewMetricsOverride}: $e', | ||
| e, | ||
| st, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
[CONCERN] Rethrowing the exception here allows callers (such as updateOverrides) to be aware of service extension failures. Currently, catching and logging the exception silently completes the future, which can lead to silent failures in the UI where a setting appears to change but the backend call actually failed.
| Future<void> _call(Map<String, String> parameters) async { | |
| final isolateGeneration = _isolateGeneration; | |
| try { | |
| final response = await serviceConnection.serviceManager | |
| .callServiceExtensionOnMainIsolate( | |
| registrations.viewMetricsOverride, | |
| args: parameters, | |
| ); | |
| if (disposed || isolateGeneration != _isolateGeneration) return; | |
| _takeOverridesFrom(response.json?['overrides']); | |
| } catch (e, st) { | |
| _log.warning( | |
| 'Failed to call ${registrations.viewMetricsOverride}: $e', | |
| e, | |
| st, | |
| ); | |
| } | |
| } | |
| Future<void> _call(Map<String, String> parameters) async { | |
| final isolateGeneration = _isolateGeneration; | |
| try { | |
| final response = await serviceConnection.serviceManager | |
| .callServiceExtensionOnMainIsolate( | |
| registrations.viewMetricsOverride, | |
| args: parameters, | |
| ); | |
| if (disposed || isolateGeneration != _isolateGeneration) return; | |
| _takeOverridesFrom(response.json?['overrides']); | |
| } catch (e, st) { | |
| _log.warning( | |
| 'Failed to call ${registrations.viewMetricsOverride}: $e', | |
| e, | |
| st, | |
| ); | |
| rethrow; | |
| } | |
| } |
References
- Prioritize logic, performance on the UI thread, and architectural consistency. (link)
| void _onAvailabilityChanged() { | ||
| final available = _hasExtension?.value ?? false; | ||
| if (available == _available.value) return; | ||
| _available.value = available; | ||
| if (!available) return; | ||
| unawaited(_queue(_read)); | ||
| unawaited(_queue(_restoreMetrics)); | ||
| } |
There was a problem hiding this comment.
[CONCERN] Since background tasks are run using unawaited, any uncaught exceptions from _queue will bubble up to the Zone's uncaught error handler. Adding .catchError ensures these background errors are safely handled and logged without crashing or triggering test failures.
void _onAvailabilityChanged() {
final available = _hasExtension?.value ?? false;
if (available == _available.value) return;
_available.value = available;
if (!available) return;
unawaited(
_queue(_read).catchError((e, st) {
_log.warning('Failed to read view metrics overrides: $e', e, st);
}),
);
unawaited(
_queue(_restoreMetrics).catchError((e, st) {
_log.warning('Failed to restore view metrics overrides: $e', e, st);
}),
);
}References
- Prioritize logic, performance on the UI thread, and architectural consistency. (link)
| Future<void> _setOverride(String metric, Object? value) async { | ||
| await _viewMetricsOverrideClient?.updateOverrides({metric: value}); | ||
| } |
There was a problem hiding this comment.
[CONCERN] Catching the exception here and pushing a notification via notificationService provides a much better user experience. It ensures that if the service extension call fails, the user is notified immediately rather than the UI silently failing to apply the override.
| Future<void> _setOverride(String metric, Object? value) async { | |
| await _viewMetricsOverrideClient?.updateOverrides({metric: value}); | |
| } | |
| Future<void> _setOverride(String metric, Object? value) async { | |
| try { | |
| await _viewMetricsOverrideClient?.updateOverrides({metric: value}); | |
| } catch (e) { | |
| notificationService.push('Failed to set override: $e'); | |
| } | |
| } |
References
- Prioritize logic, performance on the UI thread, and architectural consistency. (link)
| static double? _textScaleFactor(Object? textScaling) { | ||
| if (textScaling case [[final num unscaled, final num scaled], ...]) { | ||
| return scaled / unscaled; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
[CONCERN] Defensively check if unscaled is zero to avoid division by zero, which would result in double.infinity or double.nan and potentially cause layout or rendering issues in the UI.
| static double? _textScaleFactor(Object? textScaling) { | |
| if (textScaling case [[final num unscaled, final num scaled], ...]) { | |
| return scaled / unscaled; | |
| } | |
| return null; | |
| } | |
| static double? _textScaleFactor(Object? textScaling) { | |
| if (textScaling case [[final num unscaled, final num scaled], ...]) { | |
| return unscaled == 0 ? null : scaled / unscaled; | |
| } | |
| return null; | |
| } |
References
- Prioritize logic, performance on the UI thread, and architectural consistency. (link)
| Future<void> _overrideTextScale(double textScale) async { | ||
| await widget.controller.setTextScale(textScale); | ||
| if (mounted && _sliderTextScale == textScale) { | ||
| setState(() => _sliderTextScale = null); | ||
| } | ||
| } |
There was a problem hiding this comment.
[CONCERN] Wrapping the call in a try-finally block ensures that _sliderTextScale is reset to null even if the service call fails. This prevents the slider from getting stuck at an invalid/failed value and allows it to snap back to the actual value reported by the app.
Future<void> _overrideTextScale(double textScale) async {
try {
await widget.controller.setTextScale(textScale);
} finally {
if (mounted && _sliderTextScale == textScale) {
setState(() => _sliderTextScale = null);
}
}
}References
- Prioritize logic, performance on the UI thread, and architectural consistency. (link)
Part of #9893.
This connects the text scale, bold text, screen reader and high contrast overrides on the Accessibility page to the app. They go through
ext.flutter.viewMetricsOverride, which flutter/flutter#192764 added and flutter/flutter#194311 made take effect in the app. Next come moving brightness to this extension, and a fix for the brightness override that is not restored after a hot restart while this page is open.The new
ViewMetricsOverrideClient:Flutter.ServiceExtensionStateChangedevents. It listens to the events itself, becauseServiceExtensionManagerwould turn their JSON value into a string.On the page:
textScaling: [[1, factor]], which scales text linearly. Until text scale is overridden, the page shows "System default", because 1.0 is an override too. A reset button removes the override.boldText,accessibleNavigationandhighContrast. Turning one off removes the override, so the app goes back to the platform's setting.Caveats:
accessibilityexperiment flag.The client tests use a fake of the extension that, like the VM service, never answers a call that is in flight when its isolate exits. They cover following the app, failures and hot restarts, including one during the restore. Controller and widget tests cover what the controls send and show. The
devtools_testfakes can now emit extension events and replace the main isolate.Pre-launch Checklist
General checklist
///).Issues checklist
contributions-welcomeorgood-first-issuelabel.contributions-welcomeorgood-first-issuelabel. I understand this means my PR might take longer to be reviewed.Tests checklist
AI-tooling checklist
Feature-change checklist
release-notes-not-requiredlabel or left a comment requesting the label be added.packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md.If you need help, consider asking for help on Discord.